itest: boot piri on Postgres so the stack comes up; accept any 2xx CORS preflight - #47
itest: boot piri on Postgres so the stack comes up; accept any 2xx CORS preflight#47bajtos wants to merge 2 commits into
Conversation
TestForgeScenarios/CORS (added in #42/#43) has never passed: it asserted a matched preflight is answered 204, but versitygw renders every successful controller response — CORSOptions included — as 200 unless MetaOptions.Status says otherwise, which matches real S3 (browsers accept any 2xx for a preflight). 204 is only the status of versitygw's no-CORS-config preflight fallback, a path ingot never hits once cors_allowed_origins reports a configuration for every bucket. Contrary to the diagnosis in #45, the pinned versitygw (v0.0.0-20260716095011) already ships the preflight route and CORS middlewares; reproducing the itest's requests against an in-process server shows every header assertion (Allow-Origin echo on a wildcard match, Allow-Methods incl. PUT, Max-Age 600, disallowed origin bare, presigned-GET Expose-Headers incl. ETag) passing — the status was the sole failure, exactly the one line CI reported. Fixes #45. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017rwJ8KiPhm3G27vfU21Phk
There was a problem hiding this comment.
Pull request overview
Updates the Forge integration scenario test to expect a 200 OK response for a successful CORS preflight, aligning the test with observed S3-style behavior and typical browser expectations for OPTIONS preflight handling.
Changes:
- Update the
TestForgeScenarios/CORSpreflight status assertion from204 No Contentto200 OK. - Add an explanatory comment clarifying why
200is expected for a matched preflight.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Every top-level itest — TestForgeVersity, TestForgeScenarios, TestForgeNativeProvision, TestForgeReadAfterEviction — failed at stack boot, not in any assertion: Error: initializing piri node: ... ProvideHarmonyDB ... curio PDP pipeline requires Postgres (set database type to postgres) Since piri:main absorbed Curio (~2026-07-24) its PDP pipeline refuses sqlite, and forgeStack booted sqlite piri via the zero-value PiriNodeConfig. All four suites route through that one constructor, so flipping the topology default to Postgres:true is the whole fix; the pinned smelt already supports it (no dependency bump). Also relax the CORS preflight status assertion from 200 to any 2xx, the fetch spec's 'ok status' range. versitygw answers a matched rule 200 and only its no-CORS-config fallback 204, and the Max-Age assertion is what actually pins the real path: the fallback sets no Max-Age and mirrors the requested method instead of the rule's method list. This supersedes the exact-200 assertion in 900d6b5. Both changes match what #44 arrived at independently, so the overlap resolves trivially whichever lands first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017rwJ8KiPhm3G27vfU21Phk
| stack.WithPiriNodes(stack.PiriNodeConfig{}), | ||
| // Postgres-backed piri: piri:main's curio PDP pipeline refuses | ||
| // sqlite ("curio PDP pipeline requires Postgres") as of 2026-07-24. | ||
| stack.WithPiriNodes(stack.PiriNodeConfig{Postgres: true}), |
There was a problem hiding this comment.
It would be great to change stack.WithPiriNodes to configure Postgres by default. Perhaps as part of the larger cleanup to remove SQLite from Piri & Smelt.
There was a problem hiding this comment.
Agreed that's the right end state — but WithPiriNodes / PiriNodeConfig live in fil-forge/smelt, so flipping the default can't land in this PR. Keeping this one line here is the minimum to get a red suite green against the smelt version ingot currently pins.
Worth noting for whenever the smelt cleanup happens: this line can't rot silently either way. If smelt just flips the default, Postgres: true becomes redundant but stays correct. If the field goes away entirely along with sqlite, this becomes a compile error at bump time — so it surfaces immediately rather than quietly reverting the suite to sqlite.
Happy to pick up the smelt-side change (invert or drop the field, then drop this argument here) as a follow-up if you want to point me at it — it's outside this repo, so it needs its own PR there.
Generated by Claude Code
The subtest (added in #42/#43) asserts preflight behavior the pinned versitygw does not implement, and merged while the itest suite could not boot a stack — so it has never passed in any environment. Skip with a pointer to #45 rather than leave the suite red for a failure that belongs to the CORS feature, not whichever branch runs it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #45. The itest suite is green again — see the note below, because the root cause is not the one #45 diagnosed.
The failure was stack boot, not the CORS test
Every top-level itest failed, and none of them reached an assertion:
Since
piri:mainabsorbed Curio (~2026-07-24) its PDP pipeline refuses sqlite, andforgeStackbooted sqlite piri via the zero-valuePiriNodeConfig{}. This is pre-existing onmain: main's own last itest run (c57186b4, the #42 merge this branch is based on) fails identically — same four suites, 24 occurrences of that error.All four suites route through that one constructor, so the fix is one line:
stack.WithPiriNodes(stack.PiriNodeConfig{Postgres: true}). The pinned smelt already supports it (PiriNodeConfig.Postgres→manifest.DBPostgres), so no dependency bump is involved.Correction to the #45 diagnosis: the versitygw pin is fine
#45 concluded that the pinned
versitygw v0.0.0-20260716095011predates the CORS work and that the fix required bumping to a fork build with preflight support. That isn't the case — that exact module version already ships the whole stack:OPTIONS /:bucketand/:bucket/*routes (s3api/router.go:1547-1561)CORSOptionscontroller (s3api/controllers/options.go)ApplyBucketCORSon every bucket/object route, andApplyBucketCORSPreflightFallbackReproducing the itest's exact requests against an in-process server (real versitygw
s3api+ ingot's config→CORS-document path) shows every header assertion already passing on the current pin: Allow-Origin echoed on thehttps://*.dev.examplewildcard match, Allow-Methods including PUT, Max-Age 600, a bare 403 for a disallowed origin, and Expose-Headers including ETag on a presigned cross-origin GET. No versitygw bump is needed.The one genuine test defect was the status: versitygw's
ProcessControllerrenders every successful controller response as 200 unlessMetaOptions.Statusis set, andCORSOptionsdoesn't set it. 204 is only what the no-CORS-config fallback returns — a path ingot never takes, since it reports a CORS configuration for every bucket.So the assertion now accepts any 2xx, the fetch spec's "ok status" range. Nothing is lost in strictness: the
Max-Age == "600"check is what actually pins the real CORS path, because the fallback sets no Max-Age and mirrors the requested method instead of the rule's method list.Overlap with #44
#44 independently arrived at both of these same changes (piri-on-Postgres, and the 2xx relaxation) as incidental fixes alongside its catalog-retention work. The overlap resolves trivially whichever lands first. #44's note that
TestForgeReadAfterEvictionwould still fail on an upstream piri did:plc proof-verification problem did not reproduce here — it passed in 127.66s.Verification
itestjob green on574a0a5(run 30366470280), all four suites passing andTestForgeScenarios/CORSgenuinely running rather than skipped:Zero failures; the remaining skips are the pre-existing curated XFail rows and the documented position-dependent cases. Unit suite green on all three platforms,
gofmtclean,go vet -tags itest ./itestclean.